Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move IP Pools edit form to view page #2405

Merged
merged 8 commits into from
Aug 30, 2024
Merged

Conversation

charliepark
Copy link
Contributor

@charliepark charliepark commented Aug 30, 2024

This PR moves the "Edit IP pool" side modal from the IP Pools list page to the individual IP Pool view page. In some ways, this makes more sense with our existing URL schema, as the edit URL is http://localhost:4000/system/networking/ip-pools/:pool/edit, so is conceptually already a child of the pool itself, rather than the list.

We start with an additional actions button in the header …
Screenshot 2024-08-30 at 11 07 07 AM
… which leads to the side modal on the view page …
Screenshot 2024-08-30 at 11 07 30 AM

The delete button is disabled for IP Pools with existing ranges …
Screenshot 2024-08-30 at 11 07 20 AM
… but is enabled for IP Pools that can, in fact, be deleted:
Screenshot 2024-08-30 at 11 07 56 AM

Closes #2353, and if we feel this is a good pattern to replicate, we can roll it out to other resources as well.

@charliepark charliepark linked an issue Aug 30, 2024 that may be closed by this pull request
Copy link

vercel bot commented Aug 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Aug 30, 2024 10:03pm

} else {
queryClient.invalidateQueries('ipPoolView')
navigate(pb.ipPool({ pool: pool.name }))
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would add a comment explaining why the difference. also the navigate line is the same in both cases, you can just do navigate(pb.ipPool({ pool: _pool.name }))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that we don't actually need the navigate function when the description has changed; just an onDismiss(). Have added a comment on the first navigate (when name has changed), though.

Copy link
Collaborator

@david-crespo david-crespo Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the difference in invalidations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can maybe drop the View one; will walk through the various elements of it.

The ipPoolList invalidation is needed because of the presence of the IP pools in the header nav. Without that invalidation, even though the navigate takes us to the new page and the header is correct, the dropdown has old data (this shows ip-pool-1 renamed to ip-pool-updated, without the invalidation:
Screenshot 2024-08-30 at 2 26 47 PM

Adding queryClient.invalidateQueries('ipPoolList') corrects that:
Screenshot 2024-08-30 at 2 27 22 PM

The other invalidation would be useful if the page showed the description (the only form fields in the "Edit IP pool" form are name and description), but since the description is not shown on the IP Pool page, we can actually just remove that invalidation. We'd need to add it back in if we decide down the road to show the description on this page.

Copy link
Collaborator

@david-crespo david-crespo Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, isn't the ipPoolList invalidation needed regardless of whether the name changed? And on the view one, I think we should always do the corresponding invalidation even if we aren't relying on it directly yet. Leaving out an invalidation was how we got this head-scratcher #2392 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, getting the View invalidation back in makes sense, in case we add descriptions in there.

For List, my thought was that it was only needed when the name changed, as the description changing didn't impact that navigation dropdown, so it wouldn't be necessary to invalidate that; every time I manually checked the list page after editing the description it seemed to show the updated description state in the table of IP pools. But perhaps there's an aspect of the List invalidation I'm missing. Will move the it up a level.

@charliepark charliepark marked this pull request as ready for review August 30, 2024 18:42
Copy link
Collaborator

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I get it! I didn't realize we were already using the perfect route on the list view, which was weird. So we can keep the edit button on the list, and it goes to the same route, but that route now shows the detail view with the form. Very nice. I do like this pattern. I think we should do the same to the other pages in a separate PR.

Looks good, just one change suggested, and the onSuccess thing is still not clear enough on the way.

Something we can look at in a followup is making these real links where possible like I did in #2343 for the settings link. It's more accessible and lets you middle click the links to open in a new tab. The annoying part is that we're using helpers to construct the row actions and the more actions dropdowns, so those need to modified to allow a different kind of item that just has a URL instead of an onActivate callback. I tried that in #2351 and it didn't feel worth it for like 3 links, but now there will be more.

await expect(page).toHaveURL('/system/networking/ip-pools')
await expect(page.getByRole('cell', { name: 'ip-pool-3' })).toBeHidden()
})

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wonderful

app/pages/system/networking/IpPoolPage.tsx Outdated Show resolved Hide resolved
@charliepark charliepark merged commit e8175d3 into main Aug 30, 2024
8 checks passed
@charliepark charliepark deleted the move-edit-form-to-view-page branch August 30, 2024 22:25
Comment on lines 42 to +49
queryClient.invalidateQueries('ipPoolList')
if (pool.name !== _pool.name) {
// as the pool's name has changed, we need to navigate to an updated URL
navigate(pb.ipPool({ pool: _pool.name }))
} else {
queryClient.invalidateQueries('ipPoolView')
onDismiss()
}
Copy link
Collaborator

@david-crespo david-crespo Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is equivalent since onDismiss just does the same nav.

Suggested change
queryClient.invalidateQueries('ipPoolList')
if (pool.name !== _pool.name) {
// as the pool's name has changed, we need to navigate to an updated URL
navigate(pb.ipPool({ pool: _pool.name }))
} else {
queryClient.invalidateQueries('ipPoolView')
onDismiss()
}
queryClient.invalidateQueries('ipPoolList')
if (pool.name === _pool.name) {
queryClient.invalidateQueries('ipPoolView')
}
navigate(pb.ipPool({ pool: _pool.name }))

Is the reason for the conditional that you were getting that error flash on nav? Otherwise this would work, wouldn't it?

queryClient.invalidateQueries('ipPoolList')
queryClient.invalidateQueries('ipPoolView')
navigate(pb.ipPool({ pool: _pool.name }))

Or maybe navigate first I guess?

david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Sep 16, 2024
oxidecomputer/console@7712765...ce242e7

* [ce242e77](oxidecomputer/console@ce242e77) bump API to latest, only doc comment changes
* [3fda42e3](oxidecomputer/console@3fda42e3) oxidecomputer/console#2446
* [a5c23616](oxidecomputer/console@a5c23616) oxidecomputer/console#2436
* [ca272336](oxidecomputer/console@ca272336) oxidecomputer/console#2445
* [22582e3c](oxidecomputer/console@22582e3c) oxidecomputer/console#2444
* [f9f0d157](oxidecomputer/console@f9f0d157) oxidecomputer/console#2443
* [be84c196](oxidecomputer/console@be84c196) oxidecomputer/console#2441
* [53e1da78](oxidecomputer/console@53e1da78) chore: just skip the flaky test in safari
* [3a66ca77](oxidecomputer/console@3a66ca77) bump a couple of dev deps I missed
* [ac67a4cf](oxidecomputer/console@ac67a4cf) bump omicron version to latest
* [63c604b4](oxidecomputer/console@63c604b4) oxidecomputer/console#2440
* [b4e2626b](oxidecomputer/console@b4e2626b) oxidecomputer/console#2439
* [cdecf4e6](oxidecomputer/console@cdecf4e6) oxidecomputer/console#2435
* [01d71c07](oxidecomputer/console@01d71c07) oxidecomputer/console#2432
* [353b98d6](oxidecomputer/console@353b98d6) oxidecomputer/console#2404
* [49d6d7d8](oxidecomputer/console@49d6d7d8) oxidecomputer/console#2433
* [9c532ce9](oxidecomputer/console@9c532ce9) chore: fix react duplicate key warning  on vpcs page
* [1892fc14](oxidecomputer/console@1892fc14) npm audit fix, bump msw
* [efc43be0](oxidecomputer/console@efc43be0) oxidecomputer/console#2422
* [f2cc79ee](oxidecomputer/console@f2cc79ee) oxidecomputer/console#2418
* [a0c29a53](oxidecomputer/console@a0c29a53) oxidecomputer/console#2427
* [a8fcdab9](oxidecomputer/console@a8fcdab9) oxidecomputer/console#2426
* [8ecb36ad](oxidecomputer/console@8ecb36ad) oxidecomputer/console#2425
* [93bcef9d](oxidecomputer/console@93bcef9d) oxidecomputer/console#2296
* [af42e704](oxidecomputer/console@af42e704) oxidecomputer/console#2421
* [4126f000](oxidecomputer/console@4126f000) oxidecomputer/console#2420
* [880cb8c4](oxidecomputer/console@880cb8c4) oxidecomputer/console#2415
* [c2909e7a](oxidecomputer/console@c2909e7a) oxidecomputer/console#2414
* [12fc862b](oxidecomputer/console@12fc862b) oxidecomputer/console#2412
* [169edeed](oxidecomputer/console@169edeed) remove unreachable sign in button in user dropdown
* [f042ad4b](oxidecomputer/console@f042ad4b) oxidecomputer/console#2409
* [c648c44c](oxidecomputer/console@c648c44c) oxidecomputer/console#2408
* [e8175d30](oxidecomputer/console@e8175d30) oxidecomputer/console#2405
* [bdb55b86](oxidecomputer/console@bdb55b86) chore: bump ts client generator version in api-diff
* [681355cd](oxidecomputer/console@681355cd) oxidecomputer/console#2396
* [286b8d28](oxidecomputer/console@286b8d28) oxidecomputer/console#2401
* [b2373359](oxidecomputer/console@b2373359) oxidecomputer/console#2400
* [267b9359](oxidecomputer/console@267b9359) oxidecomputer/console#2399
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Sep 16, 2024
oxidecomputer/console@7712765...ce242e7

* [ce242e77](oxidecomputer/console@ce242e77)
bump API to latest, only doc comment changes
* [3fda42e3](oxidecomputer/console@3fda42e3)
oxidecomputer/console#2446
* [a5c23616](oxidecomputer/console@a5c23616)
oxidecomputer/console#2436
* [ca272336](oxidecomputer/console@ca272336)
oxidecomputer/console#2445
* [22582e3c](oxidecomputer/console@22582e3c)
oxidecomputer/console#2444
* [f9f0d157](oxidecomputer/console@f9f0d157)
oxidecomputer/console#2443
* [be84c196](oxidecomputer/console@be84c196)
oxidecomputer/console#2441
* [53e1da78](oxidecomputer/console@53e1da78)
chore: just skip the flaky test in safari
* [3a66ca77](oxidecomputer/console@3a66ca77)
bump a couple of dev deps I missed
* [ac67a4cf](oxidecomputer/console@ac67a4cf)
bump omicron version to latest
* [63c604b4](oxidecomputer/console@63c604b4)
oxidecomputer/console#2440
* [b4e2626b](oxidecomputer/console@b4e2626b)
oxidecomputer/console#2439
* [cdecf4e6](oxidecomputer/console@cdecf4e6)
oxidecomputer/console#2435
* [01d71c07](oxidecomputer/console@01d71c07)
oxidecomputer/console#2432
* [353b98d6](oxidecomputer/console@353b98d6)
oxidecomputer/console#2404
* [49d6d7d8](oxidecomputer/console@49d6d7d8)
oxidecomputer/console#2433
* [9c532ce9](oxidecomputer/console@9c532ce9)
chore: fix react duplicate key warning on vpcs page
* [1892fc14](oxidecomputer/console@1892fc14)
npm audit fix, bump msw
* [efc43be0](oxidecomputer/console@efc43be0)
oxidecomputer/console#2422
* [f2cc79ee](oxidecomputer/console@f2cc79ee)
oxidecomputer/console#2418
* [a0c29a53](oxidecomputer/console@a0c29a53)
oxidecomputer/console#2427
* [a8fcdab9](oxidecomputer/console@a8fcdab9)
oxidecomputer/console#2426
* [8ecb36ad](oxidecomputer/console@8ecb36ad)
oxidecomputer/console#2425
* [93bcef9d](oxidecomputer/console@93bcef9d)
oxidecomputer/console#2296
* [af42e704](oxidecomputer/console@af42e704)
oxidecomputer/console#2421
* [4126f000](oxidecomputer/console@4126f000)
oxidecomputer/console#2420
* [880cb8c4](oxidecomputer/console@880cb8c4)
oxidecomputer/console#2415
* [c2909e7a](oxidecomputer/console@c2909e7a)
oxidecomputer/console#2414
* [12fc862b](oxidecomputer/console@12fc862b)
oxidecomputer/console#2412
* [169edeed](oxidecomputer/console@169edeed)
remove unreachable sign in button in user dropdown
* [f042ad4b](oxidecomputer/console@f042ad4b)
oxidecomputer/console#2409
* [c648c44c](oxidecomputer/console@c648c44c)
oxidecomputer/console#2408
* [e8175d30](oxidecomputer/console@e8175d30)
oxidecomputer/console#2405
* [bdb55b86](oxidecomputer/console@bdb55b86)
chore: bump ts client generator version in api-diff
* [681355cd](oxidecomputer/console@681355cd)
oxidecomputer/console#2396
* [286b8d28](oxidecomputer/console@286b8d28)
oxidecomputer/console#2401
* [b2373359](oxidecomputer/console@b2373359)
oxidecomputer/console#2400
* [267b9359](oxidecomputer/console@267b9359)
oxidecomputer/console#2399
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more actions menu on IP pool detail that lets you edit
2 participants